Add px guide #1156

sebastianbenz · 2021-03-05T19:03:58Z

No description provided.

kristoferbaxter

Lots of great work here, thank you!

Left you a few nits and other items for the future.

kristoferbaxter · 2021-03-08T15:53:23Z

packages/page-experience/bin/cli.js

+const url = process.argv[2];
+
+(async () => {
+  const result = await new PageExperienceGuide().analyze(url);


Can't wait for top level await!

kristoferbaxter · 2021-03-08T15:54:29Z

packages/page-experience/lib/PageDataGatherer.js

+const parseFontfaces = require('./helpers/parseFontface');
+
+// Pixel 5 XL
+const DEFAULT_VIEWPORT = {


Nit: Is there a URL you could link to that lists dimensions for popular devices?

Additionally, do we need to provide a dpi value to puppeteer?

Afaik no DPI value needed as the default is 1. Not sure why a link would be helpful here.

kristoferbaxter · 2021-03-08T15:56:13Z

packages/page-experience/lib/PageDataGatherer.js

+ * Renders a page in Puppeteer and collects all data required for the page experience recommendations.
+ */
+class PageAnalyzer {
+  constructor(config = {}) {


Nit: Does the config parameter need types?

Added type info

kristoferbaxter · 2021-03-08T15:56:39Z

packages/page-experience/lib/PageDataGatherer.js

+    return await this.gatherPageData(page, remoteStyles);
+  }
+
+  async gatherPageData(page, remoteStyles) {


Nit: Do these params and the return need to be typed?

kristoferbaxter · 2021-03-08T15:57:30Z

packages/page-experience/lib/PageDataGatherer.js

+       * @return {Array<string>} a list of URLs
+       */
+      const collectFontPreloads = () => {
+        return Array.from(document.querySelectorAll('link[rel=preload][as=font]')).map(


Is there a separate check for invalid preloads somewhere? Perhaps a preload of a value used in a font-face declaration but not preloaded as a font?

Not yet, created an issue.

kristoferbaxter · 2021-03-08T16:01:03Z

packages/page-experience/lib/PageDataGatherer.js

+        !request.url().startsWith('https://cdn.ampproject.org')
+      ) {
+        // Only donwload AMP runtime scripts as they're need for layouting the page
+        // Once self-hosting is a thing we'll have to change this


Is there a flag for when the CLI tools know self-hosting is valid?

Not yet, will add later

kristoferbaxter · 2021-03-08T16:02:43Z

packages/page-experience/lib/PageExperienceGuide.js

+   * @private
+   */
+  async loadChecks() {
+    const jsFileExtension = '.js';


Does this work with the recently launched module mode?

This loads files from the filesystem which we control.

kristoferbaxter · 2021-03-08T16:04:42Z

packages/page-experience/lib/helpers/parseFontface.test.js

+}
+`;
+  parseFontface(styles, 'http://example.com');
+});


For completeness should there be a check with subset fonts?

kristoferbaxter · 2021-03-08T16:05:57Z

packages/page-experience/lib/helpers/parseFontfaceSrc.js

+const fontfaceSrcParser = require('css-font-face-src');
+
+/**
+ * Extracts the best fontface URL for preloading (woff2 > woff > ?).


Smart here: I was naively thinking the result would always be woff2 but this code correctly validates if there are other formats available to use.

kristoferbaxter · 2021-03-08T16:06:42Z

packages/page-experience/prettier.config.js

@@ -0,0 +1,39 @@
+/**
+ * Copyright 2019 The AMP HTML Authors. All Rights Reserved.


removed the file as no longer needed

* Fixed JSDocs * More tests

patrickkettner · 2021-03-12T13:11:13Z

packages/page-experience/README.md

+  const pageExperienceGuide = new PageExperienceGuide();
+  const result = pageExperienceGuide.analyze('https://amp.dev');
+  console.log('result')
+})


is there a reason this is wrapped in () but not invoked?

not at all...fixed

patrickkettner · 2021-03-12T13:13:58Z

packages/page-experience/lib/PageDataGatherer.js

+   */
+  async execute(url) {
+    if (!this.started) {
+      throw new Error('Puppeteer not running, please call `start` first.');


is there any reason you don't just call start?

You're right, but somehow I preferred the more explicit symmetry of calling start and shutdown. It's an internal API though so I don't really care...

patrickkettner · 2021-03-12T13:21:15Z

packages/page-experience/lib/PageDataGatherer.js

+      ) {
+        // Only donwload AMP runtime scripts as they're need for layouting the page
+        // Once self-hosting is a thing we'll have to change this
+        // TODO: investigate whether we could cache these locally


You could catch them https://theheadless.dev/posts/request-interception/, and reply with local filesystem?

that was my idea as well

patrickkettner · 2021-03-12T13:22:41Z

packages/page-experience/lib/PageDataGatherer.js

+      return request.continue();
+    });
+
+    // Collect external stylesheets from requests as we can't read them otherwise due to CORS


Is there a way to miss some stuff as a result of this? Any reason not to just disable CORS in rendering options?

That didn't work for me.

patrickkettner · 2021-03-12T13:32:16Z

packages/page-experience/lib/checks/fontPreloading.js

+    }
+    if (!pageData.fontPreloads.includes(fontface.mainSrc)) {
+      if (fontface.mainSrc.startsWith('https://fonts.gstatic.com')) {
+        result.warn({


would it make sense to extract these text blobs into an external file?

patrickkettner · 2021-03-12T13:32:40Z

packages/page-experience/lib/checks/fontDisplay.js

+
+    if (!fontface.fontDisplay) {
+      result.warn({
+        title: 'Improve LCP using `font-display: optional`',


Would it make sense to extract the warnings into an external file?

For translation?

patrickkettner · 2021-03-12T13:33:06Z

comments and nits, nothing blocking a LGTM though

sebastianbenz · 2021-03-12T15:05:06Z

Thanks for the reviews!

v2.8.0-canary.4

6226a79

sebastianbenz force-pushed the add-px-guide branch from 3682c73 to d6e9c49 Compare March 5, 2021 19:12

add page experience guide

8f7e159

sebastianbenz requested a review from patrickkettner March 5, 2021 19:16

sebastianbenz marked this pull request as ready for review March 5, 2021 19:19

sebastianbenz requested a review from kristoferbaxter March 5, 2021 19:23

kristoferbaxter reviewed Mar 8, 2021

View reviewed changes

This was referenced Mar 10, 2021

page-experience: check for invalid font preloads #1157

Open

page-experience: support self-hosted runtimes #1158

Open

Addressed comments

6ba0f47

* Fixed JSDocs * More tests

sebastianbenz force-pushed the add-px-guide branch from d6e9c49 to 6ba0f47 Compare March 10, 2021 21:48

sebastianbenz requested a review from kristoferbaxter March 10, 2021 21:48

patrickkettner approved these changes Mar 12, 2021

View reviewed changes

sebastianbenz added 2 commits March 12, 2021 15:27

fix code snippet

0a2dd1f

set mobile viewport

4a34bb5

sebastianbenz force-pushed the add-px-guide branch from ae1425c to 4a34bb5 Compare March 12, 2021 14:51

sebastianbenz merged commit e7c2d16 into main Mar 12, 2021

sebastianbenz deleted the add-px-guide branch March 12, 2021 15:05

sebastianbenz added the enhancement label Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add px guide #1156

Add px guide #1156

sebastianbenz commented Mar 5, 2021

kristoferbaxter left a comment

kristoferbaxter Mar 8, 2021

sebastianbenz Mar 10, 2021

kristoferbaxter Mar 8, 2021

kristoferbaxter Mar 8, 2021

sebastianbenz Mar 12, 2021

kristoferbaxter Mar 8, 2021

sebastianbenz Mar 10, 2021

kristoferbaxter Mar 8, 2021

kristoferbaxter Mar 8, 2021

sebastianbenz Mar 10, 2021

kristoferbaxter Mar 8, 2021

sebastianbenz Mar 12, 2021

kristoferbaxter Mar 8, 2021

sebastianbenz Mar 8, 2021

kristoferbaxter Mar 8, 2021

sebastianbenz Mar 10, 2021

kristoferbaxter Mar 8, 2021

kristoferbaxter Mar 8, 2021

sebastianbenz Mar 10, 2021

patrickkettner Mar 12, 2021

sebastianbenz Mar 12, 2021

patrickkettner Mar 12, 2021

sebastianbenz Mar 12, 2021

patrickkettner Mar 12, 2021

sebastianbenz Mar 12, 2021

patrickkettner Mar 12, 2021

sebastianbenz Mar 12, 2021

patrickkettner Mar 12, 2021

patrickkettner Mar 12, 2021

sebastianbenz Mar 12, 2021

patrickkettner commented Mar 12, 2021

sebastianbenz commented Mar 12, 2021

		@@ -0,0 +1,39 @@
		/**
		* Copyright 2019 The AMP HTML Authors. All Rights Reserved.

Add px guide #1156

Add px guide #1156

Conversation

sebastianbenz commented Mar 5, 2021

kristoferbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickkettner commented Mar 12, 2021

sebastianbenz commented Mar 12, 2021